Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Allow coercing non-capturing closures to function pointers. #1558

Merged
merged 4 commits into from
Feb 14, 2017
Merged

RFC: Allow coercing non-capturing closures to function pointers. #1558

merged 4 commits into from
Feb 14, 2017

Conversation

archshift
Copy link
Contributor

@archshift archshift commented Mar 25, 2016

Rendered

Closes #1555.

@archshift archshift changed the title Add RFC 'closure_to_fn_coercion' — Allow coercing non-capturing closures to function pointers. Allow coercing non-capturing closures to function pointers. Mar 25, 2016
@archshift
Copy link
Contributor Author

This is my first RFC — should I modify 0401-coercions as well?

EDIT: After a short conversation on #rust-internals, I've included a short delta to that RFC.

@archshift archshift changed the title Allow coercing non-capturing closures to function pointers. RFC: Allow coercing non-capturing closures to function pointers. Mar 25, 2016
@bstrie
Copy link
Contributor

bstrie commented Mar 26, 2016

I much prefer the version that doesn't introduce any new syntax.

How would this interact with the existing closure types? Would the following code compile?

fn main() {
    let x = |var: &mut u32| {};
    foo(&x);
    bar(x);
}

fn foo(_: &Fn(&mut u32)) {}

fn bar(_: fn(&mut u32)) {}

I suppose what this is asking is, is the type of x actually a function pointer that just so happens to be using the closure syntax, or is it a closure that just so happens to be coercible to a function pointer? If it's the former, then should we expect to have to use the following syntax, in the same way that trait objects do today?

fn main() {
    let x: fn(&mut u32) = |var| {};
    // alternatively:
    let x = |var| {} as fn(&mut u32);
    bar(x);
}

fn bar(_: fn(&mut u32)) {}

@archshift
Copy link
Contributor Author

Whether a closure's actual type becomes fn when it does not capture can theoretically be an implementation detail, because fn already satisfies the Fn trait anyway. This is, of course, barring the programmer taking any unsafe liberties with the structure of a closure object.

For example, this will compile and run fine:

fn foobar(_: &mut u32) {}

fn main() {
    let x = foobar;
    foo(&x);
    bar(x);
}

fn foo(_: &Fn(&mut u32)) {}

fn bar(_: fn(&mut u32)) {}

Strictly according to the RFP, though, the variable would be a closure that later coerces to a function pointer, which is probably for the better to prevent any breakage of silly unsafe code.

@bstrie
Copy link
Contributor

bstrie commented Mar 26, 2016

the variable would be a closure that later coerces to a function pointer

Does the type system need some way to distinguish between normal closures and "bare" closures? Can we achieve all this without runtime checks?

@archshift
Copy link
Contributor Author

Yes, ideally the type system would keep track of closures that do not capture their environment.

Runtime checking is not a solution at all, in my opinion, nor is it necessary.

@bstrie
Copy link
Contributor

bstrie commented Mar 26, 2016

Am I right in that there would be no semantic difference between the existing fn type and a hypothetical "bare closure" type? If that's the case then it seems like the simplest route would just be to have Rust automatically assign zero-size closures a fn type (I had forgotten that fn satisfies Fn). But you say that this might be a problem for unsafe code, can you give an example of what sort of unsafe code you're worried would break under this scheme?

@archshift
Copy link
Contributor Author

After a quick check of rust's source, you're right. There really isn't any way to transmute a non-capturing closure (TIL it's zero-sized!), so no unsafe code could be broken by this.

In that case, there wouldn't be any downside after all to simply using a fn type for such "trivial" closures.

@blaenk
Copy link
Contributor

blaenk commented Mar 28, 2016

I remember I used to talk about this with cc @P1start over a year ago; I think @eddyb too, perhaps regarding bounds-targetting coercions? I don't remember.

It'd be nice to have in some form. I also would prefer it to be automatic if possible.

@eddyb
Copy link
Member

eddyb commented Mar 28, 2016

This can all be done easily at the type level: allow coercions from closures with no captures (a fact which is recorded in their very type).

The coercion you want is called a "reification" coercion and it's what turns functions into fn pointers today (you can treat non-closure fns as closures with no captures if you want to).

As for trans, well, the fn pointer is just <typeof(closure) as FnOnce<_>>::call_once (as zero-sized arguments are always ignored now), so that's a small bit of plumbing.

@glaebhoerl
Copy link
Contributor

FWIW I wouldn't mind having fn literals - independently of the fate of this RFC. Yeah it's niche and "more syntax", but the fn type not having a literal form is kind of a hole anyways.

@eddyb
Copy link
Member

eddyb commented Mar 28, 2016

@glaebhoerl FWIW there are no more items with fn types - they can only be produced by reification, which is what this PR would allow for closure expressions too.

@nrc nrc added the T-lang Relevant to the language team, which will review and decide on the RFC. label Mar 29, 2016
@gkoz
Copy link

gkoz commented Apr 1, 2016

[EDIT: I've realized the double boxing trick might be avoidable and boxing a ZST is a no-op, so ignore this.]

It could be useful to specialize for non-capturing closures (together with usual fns) when talking to APIs that don't understand thick pointers. Simply making || {} coercible to fn() doesn't seem sufficient though, there needs to be a way to bound on this.

trait IntoRaw {
    /// converts into a thin pointer
    fn into_raw(self) -> *mut c_void;
}

impl<F: Fn() + 'static> IntoRaw for F {
    default fn into_raw(self) -> *mut c_void {
        // box twice to get a thin pointer
        let b: Box<Box<Fn()>> = Box::new(Box::new(self));
        Box::into_raw(b) as *mut _
    }
}

impl<F: Fn() + Coercible + 'static> IntoRaw for F {
    fn into_raw(self) -> *mut c_void {
        // even a closure's code is static right?
        self as fn() as *mut _
    }
}

@withoutboats
Copy link
Contributor

This RFC is hasn't seen much activity in a long time, so it'd be good to get it resolved. I'm mildly in favor of merging, but I have concerns that are enough that I haven't asked rfcbot to fcp it. Here are my thoughts.

Breaking changes

My biggest concern is that this could make adding a captured variable to a closure a breaking change in a library; it would be very unfortunate for users to have to determine if subtle changes like that could break a downstream crate, rather than just always knowing that they're fine. To me, if this were the case, this RFC would be a no go.

Fortunately, I can't think of a way to expose a closure to clients except through the impl Fn mechanism. It seems obvious to me that an impl Fn should not coerce to an fn, even if the type it secretly is could coerce, but this should be made explicit in the RFC.

If there are no ways to expose values of the anonymous type to clients, then this should be fine. But I want to make sure we've thought about this.

Accidental guarantees

A more subtle concern is one in which the user accidentally guarantees this coercion, and then later is forced to break their API to perform the upgrade they intended.

This basically introduces two ways to return closures, one of which is strictly more general than the other:

fn incr() -> fn(i32) -> i32 {
    |x| x + 1
}

fn incr() -> impl Fn(i32) -> i32 {
    |x| x + 1
}

My concern is that the first of these is more natural to write than the second. Users could write functions which return fn types, not realizing that they are declaring in their API that the function they return captures no variable from the environment.

This seems a pretty serious trade off to me - we want the language to just enable you to do the right thing. The current behavior is unergonomic and confounding, but I'm not 100% sure that the footguns this behavior would introduce are less of a problem.

@glaebhoerl
Copy link
Contributor

@wycats So have fn literals instead? 😁

@withoutboats
Copy link
Contributor

@glaebhoerl assuming that @ was at me 😉, I think the downsides of a new lambda syntax are greater than the downsides of the current behavior or the RFC.

@glaebhoerl
Copy link
Contributor

Oh, sorry! And I made the opposite mistake just a couple of hours ago... (but thankfully twitter does the reply-tos automatically;)

@arielb1
Copy link
Contributor

arielb1 commented Nov 5, 2016

Fortunately, I can't think of a way to expose a closure to clients except through the impl Fn mechanism. It seems obvious to me that an impl Fn should not coerce to an fn, even if the type it secretly is could coerce, but this should be made explicit in the RFC.

impl Fn types are hidden just as much as generic type parameters.

@archshift
Copy link
Contributor Author

archshift commented Nov 7, 2016

@withoutboats

It seems obvious to me that an impl Fn should not coerce to an fn, even if the type it secretly is could coerce, but this should be made explicit in the RFC.

For what it's worth, I agree, and I've added a comment to the RFC making that explicitly clear.

A more subtle concern is one in which the user accidentally guarantees this coercion, and then later is forced to break their API to perform the upgrade they intended.

I agree that this could happen, and I've added this to the drawbacks section of the RFC, but we do already have situations in which a Rust programmer might take the "easy route" and accidentally constrain their own API. (See example in RFC.) I do think that to some extent, we expect a Rust programmer to be aware of the limitations they create for themselves.

In this case we're talking about a much more niche feature than mut, of course, but I think the benefits to code clarity and brevity outweigh some accidental misuse.

@withoutboats
Copy link
Contributor

withoutboats commented Nov 11, 2016

@rfcbot fcp close

We discussed this at the lang meeting this week. We considered both the coercion option the RFC proposed and the alternative function literal syntax, as well as allowing only explicit as casts of closures into fn types, but ultimately we felt that there was not strong enough motivation to adopt any of these.

In general, folks felt most favorably about the function syntax (though speaking personally I think I'm more against it than other people were), but there were concerns about how it would work. If the function syntax infers your types (especially the return type), it behaves significantly differently from named function declarations. If it doesn't, it doesn't save you much in comparison to writing out a named function.

No one loves the current situation, but we felt that the language as it exists right now does not strongly encourage you to use fn pointers, so its not clear how often some new syntax or coercion would actually be used for its intended purpose.

@rfcbot
Copy link
Collaborator

rfcbot commented Nov 11, 2016

Team member @withoutboats has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@nikomatsakis
Copy link
Contributor

@withoutboats

I am concerned about the ability to get yourself into a bind with it where you need to add some environment to your function and you can't.

Hmm, it seems like this is fundamentally a judgement call. There are definitely legitimate cases where you actively want a function pointer and not a closure (e.g., interfacing with C code, as well as wanting to guarantee that there is no environment, though that latter case could be achieved by adding some kind of "zero-sized" trait). But making those cases more ergonomic comes at the cost of making it easier to write the wrong thing in your public APIs.

I have to admit I am not so worried about that. I think beginners probably will make mistakes, but as they learn rust, they will learn the better fix, and their APIs will improve and be updated. It seems likely that many early APIs also make dubious choices about whether to take ownership of something or to borrow it, and so forth, as well.

@aturon
Copy link
Member

aturon commented Jan 11, 2017

@nikomatsakis I agree. I'd also note that fn as a type is not something that people get exposed to early on in learning Rust, so I'm not sure how much temptation there would be to use it in APIs before one understands the implications.

I'm going to go ahead and move toward acceptance of this RFC, given recent discussion, but of course @withoutboats and others should feel free to raise concerns.

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jan 11, 2017

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@archshift
Copy link
Contributor Author

@withoutboats
Regarding accidentally trapping oneself API-wise, I believe distinguishing between a fn() and a closure is akin to distinguishing between Fn/FnMut/FnOnce. (The traits, I believe, are much easier to misuse as well.)

As long as we sufficiently emphasize the fact that function pointers are completely self-contained, I don't anticipate this to be a problem.

@aturon
Copy link
Member

aturon commented Jan 21, 2017

Ping @nrc @pnkfelix @withoutboats

@aturon aturon removed the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Jan 23, 2017
@withoutboats
Copy link
Contributor

I've marked this reviewed to merge. I think my biggest concern has to do with stabilizing this before impl Trait - people might start moving to fn pointers because they don't want to use Box<Fn>.

@pnkfelix
Copy link
Member

pnkfelix commented Feb 1, 2017

I have marked this reviewed to merge, but I have to admit that I was confused when re-reading the Summary sentence:

A non-capturing (that is, does not Clone or move any local variables) closure
should be coercable to a function pointer (fn).

What's up with that definition of "non-capturing"? I would have thought the definition would be broader, to include referencing local variables (even without moving nor cloning them).

(And I'm pretty sure that that broader definition of "non-capturing" must be what everyone else is using, because the only way the proposal makes sense is for the generated code to not have any dynamic environment that it needs to access...)

@eddyb
Copy link
Member

eddyb commented Feb 1, 2017

Yeah, "capture by reference" is pretty standard at this point, the parenthesis is the confusing bit there IMO.

@rfcbot
Copy link
Collaborator

rfcbot commented Feb 1, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label Feb 1, 2017
@archshift
Copy link
Contributor Author

Ah, that was an oversight. I'll fix the summary.

@rfcbot
Copy link
Collaborator

rfcbot commented Feb 11, 2017

The final comment period is now complete.

@aturon
Copy link
Member

aturon commented Feb 11, 2017

The final comment period has elapsed without any additional commentary, so at this point we're pretty much ready to merge the RFC.

However, I noticed that @nikomatsakis has an outstanding review request and I want to make sure he feels the issue has been resolved at this point.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Feb 13, 2017

@aturon @eddyb

However, I noticed that @nikomatsakis has an outstanding review request and I want to make sure he feels the issue has been resolved at this point.

Ah, hmm, yes I forgot about that. So my concern was that I would expect this coercion (from closure to fn ptr) to be driven by the top-down coercion process. I wrote this:

My expectation is that it uses the 'expected type' -- meaning that it's a kind of coercion. Basically, when we type-check a closure expression, if the expected type is a fn(), we will coerce to a fn pointer, but otherwise we will not. This implies the usual limitations that come along with coercions (at least today). These are somewhat stronger than what you mention. For example, once a closure is assigned to a variable, it will no longer be possible to coerce it to a fn() pointer.

And @aturon replied:

Is that stronger limitation fundamental? It seems to me that we could introduce a marker trait, or some other way of knowing that a particular struct is coercible to a fn pointer.

Then @eddyb wrote:

That... limitation strikes me as false. What is the difference between the type of || ... and the type of x? The coercion goes from TyClosure to TyFnPtr, doesn't it?

I suppose that thinking about it more there is another alternative. We could consider this a variant of upvar inference (the thing that decides, based on how it uses its upvars, when a closure implements Fn, FnMut, or FnOnce). Basically if a local closure type (defined in this fn, which at the moment is always the case I think) is coerced to a fn ptr, then we add a constraint that it can have no upvars.

I'd be ok with that, I guess, though I'm not totally psyched about it, just because I feel like it's yet another ad-hoc bit of obligation tracking we'd be doing in the type-checker (rather than having it all consolidated into a new-and-improved trait system sort of thing). Just examining the expected type at the time when we create the closure feels way easier.

But yeah I can go either way I guess. It's fine.

@eddyb
Copy link
Member

eddyb commented Feb 13, 2017

then we add a constraint that it can have no upvars.

The set of "upvars" is already known before type-checking begins, so that shouldn't be a problem.

@aturon
Copy link
Member

aturon commented Feb 14, 2017

Per post-FCP discussion, this RFC has been merged!

Tracking issue

@archshift archshift deleted the closure-to-fn-coercion branch February 14, 2017 19:06
@kennytm kennytm mentioned this pull request Feb 20, 2017
@Centril Centril added A-typesystem Type system related proposals & ideas A-expressions Term language related proposals & ideas A-closures Proposals relating to closures / lambdas. A-coercions Proposals relating to coercions. A-function-pointers Proposals relating to function pointers. labels Nov 23, 2018
@AaronKutch
Copy link

The "Rendered" link is broken

@shepmaster
Copy link
Member

Accepted RFCs are available at https://rust-lang.github.io/rfcs/1558-closure-to-fn-coercion.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Proposals relating to closures / lambdas. A-coercions Proposals relating to coercions. A-expressions Term language related proposals & ideas A-function-pointers Proposals relating to function pointers. A-typesystem Type system related proposals & ideas final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow coercing non-capturing closures to function pointers